Skip to content

security: an allowlisted reader must not auto-run outside the granted roots - #310

Open
Saidheerajgollu wants to merge 1 commit into
andrewyng:mainfrom
Saidheerajgollu:shell-read-scope
Open

security: an allowlisted reader must not auto-run outside the granted roots#310
Saidheerajgollu wants to merge 1 commit into
andrewyng:mainfrom
Saidheerajgollu:shell-read-scope

Conversation

@Saidheerajgollu

Copy link
Copy Markdown

Fixes #309.

Summary

read_file refuses to read outside the granted roots. run_shell does not, so an allowlisted cat returns exactly what the file tool declines to hand over, with no approval prompt:

read_file(path="<secret>/id_rsa")   -> {'error': 'path escapes the workspace'}
cat <secret>/id_rsa                 -> auto-runs, contents returned

Being on the allowlist says the program is safe to run unattended. It says nothing about where it is pointed, and nothing else covers that either: evaluate path-scopes WRITE_LOCAL by inspecting arguments["path"], which the shell has no equivalent of.

This withholds auto-run when an allowlisted command names a path outside every granted root. Withheld, not denied - the command still runs once the user approves it, so a false positive costs one prompt.

Approach

The engine already has the right helper. _under_root resolves a path against every granted root, not just the workspace:

def _under_root(self, path: str) -> bool:
    candidate = self._candidate(path)
    for rp, _ in self._resolved_roots():

rg '_under_root\b' returns one hit - the definition. It has never been called; only _under_writable_root is wired up, for writes. This wires the read side up rather than introducing a parallel notion of confinement.

Same boundary as read_file, and the same one #289 is applying to the browser tools and email_tools already applies to outgoing attachments.

Keeping the false-positive rate at zero

The check only looks at argument tokens that genuinely name a location, which is what keeps ordinary use intact:

  • patterns and subcommands are not paths - '*.py', status, install, HEAD~1 are ignored
  • a relative path resolves against the workspace, so it stays inside unless it climbs out with ..
  • _under_root resolves symlinks, so a link planted in the workspace cannot launder a target outside it
  • --file=/etc/passwd is read off the right of the =, rather than skipped as a flag
escape shapes still auto-running 0 / 11
ordinary reads regressed 0 / 16

Escapes covered: /etc/passwd, ~/.ssh/id_rsa, ../../../etc/passwd, head -n 5 /etc/hosts, wc -l /etc/passwd, grep -r AKIA /Users, ls /, pytest --collect-only /tmp/evil_test.py, an absolute path to a directory outside the roots, the in-workspace symlink, and grep --file=/etc/passwd.

Unaffected: ls -la, pwd, git status -s, git log --oneline -5, git diff --stat, grep -n TODO README.md, grep -rn TODO src/, find . -name '*.py', find . -type f -maxdepth 2, cat README.md, cat ./src/app.py, wc -l src/app.py, pytest -q, an absolute path inside the workspace, and reads inside a second granted root.

Scope

Auto-run only. Nothing here changes what the user can approve, what auto mode does, or what happens once a command is in session_allow_commands. The added condition sits on one branch of evaluate and falls through to the existing approval path, so no other decision route changes.

I kept the decision reason generic rather than returning a specific "outside the session directories" string, to keep the diff to one condition. Happy to surface the offending path in the reason if you'd rather the approval card explain itself - it is a two-line change.

Relationship to #307 / #308

Independent, and deliberately on a separate branch off main so they can merge in either order. #308 is about which program may run unattended; this is about where it may be pointed. Different axis, no overlap in the diff.

They do compose: #308 notes that docs/config.example.toml recommends cat, grep, find and ls, and that a trusted workspace can append to allowed_commands. This change is what makes that allowlist safe to hold readers at all.

It also closes the one case I explicitly carved out of #307 as out of scope - pytest /tmp/evil_test.py - because that escape is a path, and this is the axis that can express it.

Test plan

  • 21 new cases in tests/test_permissions_risk.py, alongside the existing allowlist tests: reads outside the roots (parametrized over 9 shapes), the symlink case, reads inside the workspace and inside a second granted root (parametrized over 10), and absolute paths inside a root
  • pytest tests/test_permissions_risk.py - 51 passed, no existing test modified
  • Full suite: 938 passed. The 6 tests/test_fake_slack.py failures and test_bedrock_provider.py are pre-existing on main (missing messaging / bedrock extras, Telegram Connector Issue #257 and setup_dev_env.sh misses the bedrock extra — fresh dev env fails 7 tests out of the box #284) and unrelated.

… roots

`read_file` refuses to leave the workspace - `target.relative_to(root)`, else "path
escapes the workspace" - and it refuses traversal too. `run_shell` applies no such
check, so with `cat` on the allowlist the same bytes come back with no approval prompt:

    read_file(path="/Users/me/.ssh/id_rsa")  -> {"error": "path escapes the workspace"}
    cat /Users/me/.ssh/id_rsa                -> auto-runs, contents returned

Being on the allowlist says the PROGRAM is safe to run unattended. It says nothing
about where that program is pointed, and today nothing else does either: `evaluate`
path-scopes WRITE_LOCAL by inspecting `arguments["path"]`, which the shell does not
have. The engine already carries the right helper for this - `_under_root`, resolving
against every granted root - but nothing has ever called it. This wires it up.

An allowlisted command is checked for argument tokens that name a location, and
auto-run is withheld if any of them resolves outside every granted root. Withheld, not
denied: the command still runs once the user approves it, so the cost of a false
positive is one prompt.

Only tokens that genuinely look like paths are considered, so patterns and subcommands
(`'*.py'`, `status`, `install`) are ignored and ordinary use is untouched: relative
paths resolve against the workspace and stay inside it unless they climb out with `..`.
`_under_root` resolves symlinks, so a link planted in the workspace cannot launder a
target outside it, and `--file=/etc/passwd` is read off the right of the `=` rather
than skipped as a flag.

Closes the shell hole in the same class as andrewyng#189 and andrewyng#293 (tools reaching outside the
session roots), for the tool with the widest reach.

21 cases: reads outside the roots, the symlink and `--opt=value` shapes, reads inside
the workspace and inside a second granted root, and patterns/subcommands that must not
be mistaken for paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] read_file is confined to the granted roots; an allowlisted cat via run_shell is not

1 participant